Skip to content

Do not use logging#956

Open
dandavison wants to merge 10 commits intomainfrom
no-logging
Open

Do not use logging#956
dandavison wants to merge 10 commits intomainfrom
no-logging

Conversation

@dandavison
Copy link
Contributor

@dandavison dandavison commented Feb 28, 2026

Fixes #567

What was changed

  • When executing short-lived transactional commands (e.g. start, list etc, as opposed to the long-running server start-dev process) report errors/warnings as unstructured plain text instead of structured logging-formatted messages.
  • The logger is now used only by the server and SDK

Why?

CLIs should report errors/warnings by printing to stderr. They should not use structured logging messages for this.

How was this tested

  • New in-codebase tests
  • Manually.

A script to manually repro/verify this change is in the git history of this branch. It shows, for example

Scenario: Try to connect to unavailable port

Before

time=2026-03-01T13:05:37.748 level=ERROR msg="failed reaching server: connection error: desc = \"transport: Error while dialing: dial tcp 127.0.0.1:1: connect: connection refused\""

After

Error: failed reaching server: connection error: desc = "transport: Error while dialing: dial tcp 127.0.0.1:1: connect: connection refused"

Scenario: setting env kv pairs

Before

time=2026-03-01T13:05:37.982 level=INFO msg="Setting env property" env=myenv property=foo value=bar
 time=2026-03-01T13:05:37.982 level=INFO msg="Writing env file" file=/path/to/my/env/file

After
[no output]

@dandavison dandavison force-pushed the no-logging branch 2 times, most recently from ab35e4e to f6ccd12 Compare March 1, 2026 18:33
@dandavison dandavison marked this pull request as ready for review March 1, 2026 19:00
@dandavison dandavison requested review from a team as code owners March 1, 2026 19:00
@dandavison dandavison requested a review from chaptersix March 1, 2026 19:04

if len(args) > 0 {
cctx.Logger.Warn("Passing the namespace as an argument is now deprecated; please switch to using -n instead")
fmt.Fprintln(cctx.Options.Stderr, "Warning: Passing the namespace as an argument is now deprecated; please switch to using -n instead")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why keep the cctx.Logger at all If the cli should not produce structured output?

Copy link
Contributor

@chaptersix chaptersix Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or could you just modify the logger to produce plain text on the correct stream in plain text?

Copy link
Contributor Author

@dandavison dandavison Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it didn't go far enough. I've got rid of the remaining logging, converting one into an unconditional warning. The logger is now only used by the SDK and the server.

@chaptersix
Copy link
Contributor

chaptersix commented Mar 7, 2026

is there any CI checks that could be put in place to enforce this?

@dandavison dandavison force-pushed the no-logging branch 2 times, most recently from 3d16117 to 5ff9402 Compare March 20, 2026 03:43
@dandavison
Copy link
Contributor Author

is there any CI checks that could be put in place to enforce this?

I added a comment to the logger in the struct saying that it is for SDK and server only. Hopefully people won't be tempted to use a logger seeing as it's a CLI. We could also give the logger field in the struct an off-putting name?

@dandavison dandavison requested a review from chaptersix March 20, 2026 03:46
The Fail callback was routing errors through the slog logger, which meant:
- Errors appeared as structured log messages (with timestamp and level)
- --log-level never silently swallowed errors
- --log-format json wrapped errors in JSON

Errors are now always printed directly to stderr as "Error: <message>",
independent of log configuration.

Default log level changed from "info" to "never"; server start-dev
continues to default to "warn" via its existing ChangedFromDefault
override. Users can still opt in with --log-level on any command.

Two deprecation warnings (namespace arg, env command args) converted
from logger calls to direct stderr writes so they remain visible
regardless of log level.
Convert CLI diagnostic messages (env/config file operations, env-var
flag overrides) from structured slog output to plain-text stderr gated
on --verbose. The structured logger now exists solely for the SDK client
and dev server, which conventionally use it.
--verbose should reveal non-obvious behavior (e.g. env var overriding
a flag), not echo what the user just asked to happen.
Set cctx.Verbose before populateFlagsFromEnv so it can call
printVerbose immediately, eliminating the deferred callback.
The only printVerbose call site was unreachable (the sole flag with an
env var annotation is --env, which is consumed before
populateFlagsFromEnv runs). Replace with an unconditional warning to
stderr so it works if BindFlagEnvVar is used for other flags in future.
@dandavison dandavison changed the title Decouple error reporting from logging Do not use logging Mar 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Print to stderr instead of using a logger for warnings/errors

2 participants